Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextViewer: use Throttler for firePostSelectionChanged #1695 #1703

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Feb 20, 2024

Avoids a 500ms delay after changing the selection, like higlighting the
selected word.

#1695

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious in general here if there needs to be a check that the widget is not disposed? Maybe the getDisplay() != null) check was implicitly doing that in the old logic?

@vogella
Copy link
Contributor

vogella commented Feb 20, 2024

Thanks @jukzi. Please extend the commit message, "immediately calls firePostSelectionChanged by chance." is not very telling. The project guidelines are that the commit message should describe the change and not the ticket.

Copy link
Contributor

github-actions bot commented Feb 20, 2024

Test Results

   917 files     917 suites   36m 6s ⏱️
 7 452 tests  7 300 ✅ 151 💤 1 ❌
21 932 runs  21 545 ✅ 386 💤 1 ❌

For more details on these failures, see this check.

Results for commit 1474134.

♻️ This comment has been updated with latest results.

@jukzi jukzi force-pushed the TextViewer_use_Throttler branch from 7ff0111 to dea25b2 Compare February 20, 2024 10:01
@jukzi
Copy link
Contributor Author

jukzi commented Feb 20, 2024

I'm curious in general here if there needs to be a check that the widget is not disposed? Maybe the getDisplay() != null) check was implicitly doing that in the old logic?

done - good catch.

@jukzi
Copy link
Contributor Author

jukzi commented Feb 20, 2024

Please extend the commit message

done. ok? Otherwise please make a suggestion.

@jukzi jukzi force-pushed the TextViewer_use_Throttler branch from dea25b2 to 7abf4fa Compare February 20, 2024 12:30
@jukzi jukzi force-pushed the TextViewer_use_Throttler branch from 7abf4fa to 2e1abe1 Compare February 21, 2024 10:58
@jukzi jukzi force-pushed the TextViewer_use_Throttler branch from 2e1abe1 to 976f3c7 Compare March 5, 2024 07:10
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge it as soon as CI pass so we get it in early in 4.32 to adapt if necessary.

(related) We should also consider, in a separate PR, changing the value of OpenStrategy.getPostSelectionDelay() to some duration that better matches "a bit more than the typical duration between 2 arrow keystorkes".

@jukzi jukzi force-pushed the TextViewer_use_Throttler branch from 7523d5b to 6eff201 Compare March 5, 2024 11:49
@jukzi jukzi force-pushed the TextViewer_use_Throttler branch from 6eff201 to 1474134 Compare March 5, 2024 12:40
@jukzi jukzi merged commit bece316 into eclipse-platform:master Mar 5, 2024
14 of 16 checks passed
@jukzi jukzi deleted the TextViewer_use_Throttler branch March 5, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants